Skip to content

ARROW-4609: [C++] Use google benchmark from toolchain#3681

Closed
xhochy wants to merge 1 commit into
apache:masterfrom
xhochy:ARROW-4609
Closed

ARROW-4609: [C++] Use google benchmark from toolchain#3681
xhochy wants to merge 1 commit into
apache:masterfrom
xhochy:ARROW-4609

Conversation

@xhochy

@xhochy xhochy commented Feb 18, 2019

Copy link
Copy Markdown
Member

No description provided.

@kszucs

kszucs commented Feb 18, 2019

Copy link
Copy Markdown
Member

@xhochy CI is failing.

@wesm

wesm commented Feb 19, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

The linker command looks fishy:

: && /home/travis/build/apache/arrow/cpp-toolchain/bin/ccache /usr/lib/ccache/g++  -ggdb -O0  -Wall -Wconversion -Wno-sign-conversion -Werror -msse4.2  -B/home/travis/build/apache/arrow/cpp-toolchain/bin -g  -rdynamic src/arrow/CMakeFiles/arrow-column-benchmark.dir/column-benchmark.cc.o  -o debug/arrow-column-benchmark  -Wl,-rpath,/home/travis/build/apache/arrow/cpp-build/debug:/home/travis/build/apache/arrow/cpp-toolchain/lib debug/libarrow_benchmark_main.a debug/libarrow_testing.so.13.0.0 debug/libarrow.so.13.0.0 -ldl /home/travis/build/apache/arrow/cpp-toolchain/lib/libdouble-conversion.a /home/travis/build/apache/arrow/cpp-toolchain/lib/libboost_system.so /home/travis/build/apache/arrow/cpp-toolchain/lib/libboost_filesystem.so /home/travis/build/apache/arrow/cpp-toolchain/lib/libboost_regex.so /home/travis/build/apache/arrow/cpp-toolchain/lib/libgtest.a /home/travis/build/apache/arrow/cpp-toolchain/lib/libgmock_main.a /home/travis/build/apache/arrow/cpp-toolchain/lib/libgmock.a -ldl /home/travis/build/apache/arrow/cpp-toolchain/lib/libbenchmark.a -lpthread jemalloc_ep-prefix/src/jemalloc_ep/dist//lib/libjemalloc_pic.a -pthread -lrt -Wl,-rpath-link,/home/travis/build/apache/arrow/cpp-toolchain/lib && :

the -lpthread is wrong. Our handling of pthreads is dealt with by the Threads::Threads variable which correctly adds -pthread (@fsaintjacques was just telling me about the difference between these two things). This unfortunately is coming from gbenchmark's cmake config

set_target_properties(benchmark::benchmark PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "-lpthread"
)

This is a bug in gbenchmark. We may have to patch gbenchmark in conda-forge. See

https://stackoverflow.com/a/39547577/776560

also

@wesm

wesm commented Feb 19, 2019

Copy link
Copy Markdown
Member

I just submitted a PR to gbenchmark

google/benchmark#771

@wesm

wesm commented Feb 19, 2019

Copy link
Copy Markdown
Member

Google accepted my PR. In the meantime I think we can handle this in the conda-forge build

conda-forge/benchmark-feedstock#4

@xhochy @wesm
ARROW-4609: [C++] Use google benchmark from toolchain
f6a1819 
Change linker statement to use Threads::Threads

Change-Id: I81df95bde48a48bed221dac814836a417c9b16ff
@wesm

wesm commented Feb 20, 2019

Copy link
Copy Markdown
Member

Hm, it looks like the gbenchmark fix was not sufficient. It seems that the conda-forge packages for gbenchmark are not usable with the Ubuntu Xenial system compilers

cc @pitrou

@pitrou

pitrou commented Feb 20, 2019

Copy link
Copy Markdown
Member

libstdc++ on 18.04 (Bionic) does have the required symbols:

$ nm --defined-only --demangle /home/antoine/miniconda3/envs/pyarrow/lib/libstdc++.so.6 | \grep std::thread::_M_start_thread
00000000000b8738 T std::thread::_M_start_thread(std::shared_ptr<std::thread::_Impl_base>)
00000000000b869e T std::thread::_M_start_thread(std::shared_ptr<std::thread::_Impl_base>, void (*)())
00000000000b8576 T std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)())

Perhaps it's a matter of linker option order? Does libbenchmark.a have to come before libarrow.so?

@wesm

wesm commented Feb 20, 2019

Copy link
Copy Markdown
Member

In Travis here I think the Ubuntu 16.04 toolchain is being used

@wesm

wesm commented Feb 25, 2019

Copy link
Copy Markdown
Member

What do you want to do with this patch? Seems that using the CF packages with Ubuntu Xenial's gcc 5.4 is a non-starter

@xhochy

xhochy commented Feb 26, 2019

Copy link
Copy Markdown
Member Author

I'm addressing this in #3688

@xhochy xhochy closed this Feb 26, 2019
@asfimport asfimport mentioned this pull request Mar 14, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xhochy @kszucs @wesm @pitrou